Repository navigation
fix(mongodb): store SET values as sent in the numeric ADD pipeline update - #589
Conversation
…date
An unconditional UpdateItem on a MongoDB table with no index and no stream is
built as a native update while the server records that the table has no index.
With a numeric ADD it is an aggregation pipeline, and each SET value went into
its `$set` stage as it was. In a pipeline that value is an expression:
- A string that starts with `$` is a field path or a variable, so
`SET #s = :dollar ADD #n :one` with `:dollar` = `{"S": "$item_data"}`
stored the item_data document in `s.S`; the update and every later read of
the item answered InternalServerError, and so did every Query of its
partition and every Scan of its table. `$$NOW` stored a date and `$$REMOVE`
left `s` empty; `$`, a member named `$k` and a member whose name holds `.`
made MongoDB refuse the update.
- A document value merges into the embedded document it replaces, so a SET of
a map over a map kept the old members, and a SET that changed the
attribute's type stored two type tags and answered InternalServerError.
Each SET value in the pipeline stage is now wrapped in `$literal`, which
stores it as a constant. Amazon DynamoDB stores every SET value as sent and
replaces the attribute it overwrites, with or without the ADD (measured
2026-10-25 at 23:15 UTC and 2026-10-26 at 00:55 UTC, each case twice).
…meric ADD Dual-target wire tests from the 2026-10-25 23:15 UTC and 2026-10-26 00:55 UTC measurements: strings `$item_data`, `$$ROOT`, `$` and `$$REMOVE`, a string set and a list holding them, maps whose values hold them, maps with members named `$k`, `$literal` and names holding `.`, and the other scalar types, each stored as sent; a SET over an attribute the item already holds, including a type change; and a REMOVE in the same update as the ADD. Each test uses a table of its own and writes an item before its update, so on MongoDB the updates take the native update path.
m_dotted_member_names holds the dotted member names `a.b` and `g.h`
together with `$c.d` and `$e`, so its value contains a `$`. The new case sends
`{"M": {"a.b": {"S": "v"}}}`, a dotted member name in a value with no `$`.
In the numeric ADD pipeline stage MongoDB refuses that map unless it is
inside `$literal` (error 40183, "cannot use dotted field name 'a.b' in a sub
object"). The case pins that every SET value is wrapped, including a value
that holds no `$`: with the fix reverted, or with only the values that hold
a `$` wrapped, its update with the ADD answers InternalServerError.
try_build_native_update reads no field of the engine, so it is now an
associated function and a unit test can call it without a MongoDB
connection. The new test parses `SET #s = :dollar ADD #n :one` with
`:dollar` = `{"S": "$item_data"}`, builds the update, and checks that the
pipeline's `$set` stage holds `{"$literal": {"S": "$item_data"}}` under
`item_data.s`. The wire tests in tests/test_update_item_dollar_values.py
check the stored value, and the session path stores it as sent as well, so
they would pass if this update stopped taking the pipeline. The unit test
fails when the stage holds the bare value.
The module docstring said an unconditional update on a table with no index and no stream is built as a native update while the server records that the table has no index (60 seconds after a write). The update also stays on the session-scoped path when it asks for old values (ReturnValues ALL_OLD or UPDATED_OLD) or for consumed capacity: data_engine.rs requires `!return_old`, and crates/engine/src/update_item.rs sets it for both. The first write on the session-scoped path that finds no record, or one older than 60 seconds, reads the index catalog and records the answer; no write renews the record during its 60 seconds. The docstring now states both conditions and says which write in each test records that the table has no index.
The stage test asserted only the value whose text reads as a field path. A builder that wrapped only such values passed every test while a plain string SET over a non-string attribute with the ADD answered InternalServerError on every later read. The test now carries a plain string beside it and asserts both wrappers. The wire file's docstring says when the native path is the pipeline and how long the no-index record lasts.
robinnsc
left a comment
There was a problem hiding this comment.
The fix looks right to me. On a local MongoDB 7, using a stream-enabled table as the session-path reference, head matched on everything we tried. That covered the 16 values, every type-to-type SET over an existing attribute, nesting up to 31 levels, values near 400 KB, all five ReturnValues, a TTL table, and cold, warm and expired cache. Base differed on every pipeline case with a $ or a dotted name. The new file is 41/41 on head and 16 failed on base. 64 writers x 20 calls still end at n = 1280. The plan is the same IXSCAN {pk:1}, and there's no measurable latency change (p50 about 4.0 ms both ways over 2,000 calls).
A few small things on the description, if you want to touch it up:
data_engine.rsgrows by 36 rather than 26. The test hunk is 34 lines.- In "Not changed here", the InternalServerError for overlapping paths only happens on the native path. On the session path MongoDB applies both actions, like SQLite. Also,
SET a = {S:"x"} ADD a 1over an N used to leave the item unreadable on main, and with this change the item stays intact, so that case does change, for the better.
Three pre-existing things we ran into, none caused by this PR. Probably worth separate issues:
- The pipeline ADD goes through Decimal128, so it keeps 34 significant digits where DynamoDB keeps 38. A 38-digit
nplus 1 stores...12340000on the native path and the exact...12345679on the session path.1E+125 + 9E+125is stored natively, while the session path refuses it as an overflow. - A map member named after an extended-JSON key (
$date,$oid,$numberDecimal,$binary, ...) makes the item unreadable once stored: GetItem and Scan of the table answer 500. PutItem and the session path already did this on main. Before this PR the pipeline refused it with error 168. Now it stores it like the other paths do, so this just lines the pipeline up with the rest. - A U+0000 attribute name passes
native_attribute_name, soREMOVEof one answers 500 on the native path. #368 already covers that.
| ]); | ||
| let values = std::collections::HashMap::from([ | ||
| ("dollar".to_owned(), AttributeValue::S("$item_data".into())), | ||
| ("plain".to_owned(), AttributeValue::S("x".into())), |
There was a problem hiding this comment.
Could consider adding a non-S value here, e.g. {"M": {"$k": {"S": "v"}}}. With both values being S, a version that only wraps S values still passes this test (the wire file does catch it). Might also be cheap to assert the _v bump in the same stage while you're reading it, since dropping that insert currently passes every lib test.
| names["#n"] = "n" | ||
| values[":one"] = {"N": "1"} | ||
| expression += " ADD #n :one" | ||
| dynamodb_client.update_item( |
There was a problem hiding this comment.
Might be worth a case with ReturnValues="ALL_NEW" (or UPDATED_NEW). Those stay on the pipeline, and the returned attributes come from the findAndModify result rather than the GetItem these tests check. On base, ALL_NEW and UPDATED_NEW with the $ values gave 500s or merged maps, so the fix covers them, but nothing here pins the response. A case where the item doesn't exist yet (warm cache, pipeline finds nothing, falls through) could be useful too: on base that one failed at stage parse time with error 168.
Fixes #573.
On MongoDB, an UpdateItem that SETs a value and also ADDs a number could store something other than the value sent. A
$string was read as an expression and a map merged into the map it replaced, and several of these left an item every read answered with InternalServerError. The value is now stored as sent.What happened on main
An unconditional UpdateItem that asks for no old values and no consumed capacity, on a table with no index and no stream, is built as a native MongoDB update while the server records that the table has no index. The first write on the session-scoped read-modify-write path that finds no such record, or one older than 60 seconds, reads the index catalog and records the answer; no write renews the record during its 60 seconds. With a numeric ADD,
try_build_native_updateincrates/storage-mongodb/src/data_engine.rsbuilds an aggregation pipeline and put each SET value into its$setstage as it was. In a pipeline that value is an expression:$is a field path or a variable.{"S": "$item_data"}stored theitem_datadocument ins.S, which no longer converts back to an item (S value must be a stringin the server log), and$$NOWstored a date. A string set, a list or a map value holding such a string did the same, and a map member named$literalstored{"S": "v"}as the map, because MongoDB read{"$literal": {"S": "v"}}as the operator. The update answered InternalServerError, every later GetItem of the item did too, and so did every Query of its partition and every Scan of its table, because one unreadable item fails the page.$$REMOVEleftsas an empty document.$, a map member named$kor$add, and a map member whose name holds.made MongoDB refuse the update, which answered InternalServerError and left the item unchanged.A SET without a numeric ADD uses an operator update, where values are literal, and was not affected.
The change
Each SET value in the pipeline stage is wrapped in
$literal, which makes it a constant. The fix is one line indata_engine.rs, plus the comment beside it.try_build_native_updatereads no field of the engine and is now an associated function, so a unit test calls it without a MongoDB connection.What the service answers
Measured in us-east-1, each case twice: with
SET #s = :s ADD #n :oneon 2026-10-25 at 23:15 UTC, and without the ADD, over existing attributes and with REMOVE on 2026-10-26 at 00:55 UTC. The service stores every SET value as sent and replaces the attribute it overwrites, with or without the ADD:$item_data,$$ROOT,$,$$REMOVE, the text{"$literal": 1}, a string set and a list holding$strings, a map whose value is$item_data.pk, maps with members named$k,$literal,a.band$c.d, and N, BOOL, NULL and B values. A REMOVE in the same update as the ADD removes its attribute.Tests
tests/test_update_item_dollar_values.py: 41 dual-target wire tests. The 16 values with and without the ADD, four SETs over an attribute the item holds (map over map, nested map over nested map, a type change from S to N and from L to M) with and without the ADD, and a REMOVE in the same update as the ADD. One of the 16 values is{"M": {"a.b": {"S": "v"}}}, a dotted member name in a value that holds no$. Each test uses a table of its own and writes an item right before its update, so on MongoDB the update runs on the native path.crates/storage-mongodb/src/data_engine.rs: a unit test parsesSET #s = :dollar, #p = :plain ADD #n :onewith:dollar={"S": "$item_data"}and:plain={"S": "x"}, builds the update and checks that the pipeline's$setstage holds{"$literal": {"S": "$item_data"}}underitem_data.sand{"$literal": {"S": "x"}}underitem_data.p. The wire tests check the stored value, and the session-scoped path stores it as sent too, so they would pass if the update stopped taking the pipeline. The unit test fails when the stage holds the bare value, and when only values whose text starts with$are wrapped.Run locally with the first two commits, when the new file held 39 tests, against MongoDB 7 (single-node replica set on a loopback port):
main: 15 failed, 24 passed. The 15 are the updates with the ADD whose value holds a$string, a$or dotted member name, or overwrites an existing attribute, plus the REMOVE with the ADD; every update without the ADD passes onmain.tests/: 91 passed, 1 skipped.SET #s = :dollar ADD #n :onecalls to one key end withn= 1280 andsas sent, every call on the pipeline path.$fails the four SET-over-existing tests with the ADD, and wrapping the$$REMOVEof a REMOVE fails the REMOVE test.cargo +1.97.0 fmt --all -- --check,cargo +1.97.0 clippy --all-targets -- -D warnings,cargo test --workspaceon stable and on 1.88.0 (1,767 passed each), and the MongoDB--comprehensivesuite (331 passed).Run locally on 2026-10-28 at the head, against MongoDB 7.0.45 (single-node replica set on a loopback port) and SQLite:
., with the ADD. MongoDB refuses that stage with error 40183 (cannot use dotted field name 'a.b' in a sub object) and the update answers InternalServerError.$fails 5 tests, the four SET-over-existing tests with the ADD and that map with the ADD.m_dotted_member_names, which also holds$c.dand$e, passes under it.cargo +1.97.0 test -p extenddb-storage-mongodb --lib: 123 passed, 22 ignored. With the stage holding the bare value, the new unit test fails.cargo +1.97.0 fmt --all -- --check,cargo +1.97.0 clippy -p extenddb-storage-mongodb --all-targets -- -D warningsandcargo +1.88.0 check -p extenddb-storage-mongodb --all-targets.CI runs the new file in
run-integration,run-integration-sqlite,mongodb-pytestandcassandra-pytest.Not changed here
When SET or REMOVE and ADD in one update name the same attribute, the service answers ValidationException
Two document paths overlap. ExtendDB does not refuse it: MongoDB answers InternalServerError and SQLite applies both. That is onmainand unchanged.Files
crates/storage-mongodb/src/data_engine.rsis 4,431 lines onmainand grows by 26: three for the comment, 24 for the unit test, and one fewer for the&selfparameter. Of the 14 other open PRs that change it, 13 merge clean with this branch (checked on 2026-10-28). #473 conflicts in one hunk: it adds a condition on the line above thetry_build_native_updatecall, which this branch changes toSelf::try_build_native_update, and the resolution keeps both lines.Corrected text for the fix commit's message
The squash or merge message is filled from the commit messages; the commit on the branch is not rewritten. Its first paragraph and first bullet read:
An unconditional UpdateItem that asks for no old values and no consumed capacity, on a MongoDB table with no index and no stream, is built as a native update while the server records that the table has no index. The first write on the session-scoped read-modify-write path that finds no such record, or one older than 60 seconds, reads the index catalog and records the answer; no write renews the record during its 60 seconds. With a numeric ADD it is an aggregation pipeline, and each SET value went into its
$setstage as it was. In a pipeline that value is an expression:$is a field path or a variable, soSET #s = :dollar ADD #n :onewith:dollar={"S": "$item_data"}stored the item_data document ins.S; the update and every later read of the item answered InternalServerError, and so did every Query of its partition and every Scan of its table.$$NOWstored a date,$$REMOVEleftsempty, and a map member named$literalstored{"S": "v"}as the map;$, a member named$kand a member whose name holds.made MongoDB refuse the update.